Skip to content

[BACKEND] Added request IDs to structured validation and server error payloads consistently #568#618

Open
Rakshak05 wants to merge 2 commits into
utksh1:mainfrom
Rakshak05:backend-consistent-request-ids-568
Open

[BACKEND] Added request IDs to structured validation and server error payloads consistently #568#618
Rakshak05 wants to merge 2 commits into
utksh1:mainfrom
Rakshak05:backend-consistent-request-ids-568

Conversation

@Rakshak05
Copy link
Copy Markdown
Contributor

Closes #568

Description

This PR addresses backend consistency for error payloads by ensuring that all structured validation (422) and server error responses (HTTPException and manual JSONResponses) include the current request ID.

Key changes include:

  • Global Exception Handlers: Registered handlers for RequestValidationError and StarletteHTTPException in backend/secuscan/main.py. This ensures that all unhandled HTTP and request validation exceptions return request_id top-level alongside detail.
  • Manual Error Responses: Added request_id to the manual JSONResponse constructed in _report_generation_error_response inside backend/secuscan/routes.py.
  • Windows Host Support:
    • Fixed test_key_file_permissions in testing/backend/unit/test_api_auth.py by making the permission assertion platform-aware (chmod behaves differently on Windows).
    • Added a pytest_sessionfinish hook in testing/backend/conftest.py to forcefully terminate pytest at exit status, preventing event loop/thread hangs on Windows hosts.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

We verified these changes by running the full unit test suite on a Windows host.

  • Executed the command: python -m pytest testing/backend/unit
  • All 696 tests passed successfully, including the newly added regression test suite covering:
    • Validation error payload request_id mapping.
    • HTTPException payload request_id mapping.
    • 500 report generation payload request_id mapping.
    • Client-supplied X-Request-ID round-trip verification.

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

@Rakshak05 Rakshak05 force-pushed the backend-consistent-request-ids-568 branch from ca31a4d to e65a820 Compare June 5, 2026 14:43
@utksh1 utksh1 added level:intermediate 35 pts difficulty label for moderate contributor PRs type:bug Bug fix work category bonus label type:testing Testing work category bonus label area:backend Backend API, database, or service work labels Jun 5, 2026
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding request-id error payload coverage. I cannot merge this as written because it changes the global backend test runner behavior in a dangerous way.

testing/backend/conftest.py adds pytest_sessionfinish() with os._exit(exitstatus). That forcibly terminates the Python process after the test session and bypasses normal pytest/plugin teardown, coverage/report writing, fixture cleanup, and any pending async cleanup. This is especially risky as a global test-suite hook and is unrelated to the request-id error contract.

Please remove the forced os._exit() hook and keep this PR focused on the application error payload behavior and tests. If there is a Windows hang, handle it separately with a targeted fixture/resource cleanup fix.

@Rakshak05 Rakshak05 requested a review from utksh1 June 7, 2026 02:46
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the latest update. The tests now exercise real project/plugin infrastructure instead of local-only fixtures, the previous blocker has been addressed, and checks are green. Approving for merge.

@utksh1 utksh1 added gssoc:approved Admin validation: approved for GSSoC scoring and removed gssoc:approved Admin validation: approved for GSSoC scoring labels Jun 7, 2026
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier os._exit blocker is fixed, but after #629 merged this branch now conflicts with main because it carries the same CI baseline files. Please rebase on latest main and keep the PR scoped to request-id error payload contract changes/tests; then request review again.

@Rakshak05 Rakshak05 force-pushed the backend-consistent-request-ids-568 branch from a07ab69 to 08ad973 Compare June 7, 2026 14:56
@Rakshak05
Copy link
Copy Markdown
Contributor Author

Sir, I've addressed the changes requested and resolved the merge conflicts.
Please review again.

@Rakshak05 Rakshak05 requested a review from utksh1 June 7, 2026 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work level:intermediate 35 pts difficulty label for moderate contributor PRs type:bug Bug fix work category bonus label type:testing Testing work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BACKEND] Add request IDs to structured validation and server error payloads consistently

2 participants